-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[android] Add support for external SDK clients tokens management #14631
Conversation
61c4a2c
to
32193e4
Compare
cc: @tobrun or @LukasPaczos for a first round of comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially LGTM 👍
One note that AccountsManager#PREFERENCE_SKU_TOKEN
constant should be exposed so that it can be used downstream, without hardcoding the value.
32193e4
to
328a69e
Compare
08ce07c
to
3110363
Compare
@Guardiola31337 any progress on this PR? if we want to hit the deadline for |
3110363
to
1f91422
Compare
Sure thing! This is updated and ready for another round of 👀 cc @LukasPaczos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, minor comments below. Also, it'd be great if you could add some unit tests to the AccountsManagerTest
public static final String KEY_PREFERENCE_SKU_TOKEN = "com.mapbox.mapboxsdk.accounts.skutoken"; | ||
|
||
/** | ||
* Key used to switch SKU token management on/off in AndroidManifest.xml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I don't think we need this javadoc.
public static final String KEY_META_DATA_MANAGE_SKU_TOKEN = "com.mapbox.ManageSkuToken"; | ||
|
||
/** | ||
* Default value for KEY_META_DATA_MANAGE_SKU_TOKEN (default is on) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
|
||
private long timestamp; | ||
@Nullable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's ensure that the token is actually not null by fetching it from preferences in the constructor as well.
1f91422
to
007af2a
Compare
@LukasPaczos updated and ready for another round of 👀 |
007af2a
to
434eacd
Compare
public void checksSkuTokenExternalManagement() { | ||
SharedPreferences mockedSharedPreferences = mock(SharedPreferences.class); | ||
when(mockedSharedPreferences.getString(KEY_PREFERENCE_SKU_TOKEN, "")).thenReturn("external-sku-token"); | ||
boolean isNotManaged = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: should be called isManaged
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was on purpose for better readability when creating the AccountsManager
object https://github.com/mapbox/mapbox-gl-native/blob/434eacd60cd8bdf933894c6bfe9b4fe32ad560c5/platform/android/MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/AccountsManagerTest.java#L38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 now the variable is called isNotManaged
, and has value false
, so that should mean that it is managed. Idk, just seems a little bit off to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀 Working Effectively with Unit Tests
when writing tests you should prefer DAMP (Descriptive And Maintainable Procedures)
That includes a bunch of procedures Moving Towards Readability 😅
I highly recommend you reading that book (if you haven't already). It's 🔝 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It had a reverse effect on me though haha 🤷♂️
platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/AccountsManager.java
Show resolved
Hide resolved
platform/android/MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/AccountsManagerTest.java
Show resolved
Hide resolved
platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/AccountsManager.java
Show resolved
Hide resolved
434eacd
to
75776ae
Compare
@LukasPaczos I've answered back / addressed all your comments, this is ready for a final round of 👀 |
75776ae
to
cfc1533
Compare
platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/AccountsManager.java
Outdated
Show resolved
Hide resolved
cfc1533
to
742a021
Compare
…he tokens on their side
742a021
to
8ade647
Compare
This PR builds on top of #14404 and adds support for external SDK clients to be able to manage the tokens on their side disabling the default implementation and returning the token from
SharedPreferences
i.e. the external SDK is responsible for managing the state, rotating and storing (using the proper keys defined inAccountsManager
) the tokens.This is an initial proposal, please let me know what you think.
cc @akitchen @frederoni